Commented up some potential issues with GOOL#4404
Conversation
Just one, but it seems to be representative of one type of problem with these polymorphic implementations.
B-rando1
added
the
design
JacquesCarette
left a comment
JacquesCarette
left a comment
There was a problem hiding this comment.
Excellent questions. I commented as best I could.
| class (VariableSym r, OOTypeSym r) => OOVariableSym r where | ||
| -- Bool: False for variable, True for constant. Required by the Python renderer. | ||
| staticVar' :: Bool -> Label -> VSType r -> SVariable r | ||
| -- We should probably have a way to make this a constant that can't have an L-Value |
There was a problem hiding this comment.
Yes, a redesign of L-Values would be good.
| perm :: r (IG.Permanence r) -> Doc | ||
| binding :: r (IG.Permanence r) -> Binding | ||
|
|
||
| -- Why do we have these here on top of InterfaceCommon? |
There was a problem hiding this comment.
Search for the PRs that introduced them, I'm pretty sure there are comments there for why. There was a reason!
There was a problem hiding this comment.
I looked for getFunc from here and getMethod from InterfaceGOOL.
getFunc was renamed and moved around a few times, the earliest being when it was renamed from get to getFunc in Created 'get' Value Function combination. Before then, it seems to appear from nowhere in Finally Tagless GOOL.
getMethod also seems to appear from nowhere in Finally Tagless GOOL.
I think the history is weird for that commit because it was merged from Brooks's fork. I'll have to go there and see if I can trace it back further when I have time.
| (variableType v) (R.objVar (RC.variable o) (RC.variable v)) | ||
| objVar' (variableBind v) | ||
|
|
||
| -- This seems a bit sketchy - unwrapping everything to Doc, mixing it up, then wrapping it back to Variable |
There was a problem hiding this comment.
I think this is supposed to create something that points to an array element, and then will be used as if it were a variable.
This is likely only used on the LHS, so that a redesign of L-Values might get rid of this.
| void :: VSType r | ||
|
|
||
| -- We've established that getTypeString is a bad thing | ||
| -- getType might be required, but without global state should it be a class? |
There was a problem hiding this comment.
getType is used to do a little bit of introspection. There might be other ways to do that.
It's a class because we should always be able to do that, and implementation can be per-language.
| extVar :: Library -> Label -> VSType r -> SVariable r | ||
| arrayElem :: Integer -> SVariable r -> SVariable r | ||
|
|
||
| -- Similar here - shouldn't we be handling this on a per-language basis? |
There was a problem hiding this comment.
classes exactly let you do things per-language. Now, if you mean that each language might want to vary this signature, that's different!
| class RenderBlock r where | ||
| multiBlock :: [MSBlock r] -> MSBlock r | ||
|
|
||
| -- This assumes that the renderers encode Body as Doc |
There was a problem hiding this comment.
Right, this assumes that the only thing to ever be done with a Block will be to render it. This might not be a good assumption.
| type VSUnOp a = VS (a (UnaryOp a)) | ||
|
|
||
| -- Why do we have these, when we also have NumericExpression in InterfaceCommon? | ||
| -- It looks like the implementations for NumericExpression link here, but why? |
There was a problem hiding this comment.
I don't remember. Sleuth through the PRs that introduce this?
|
|
||
|
|
||
| -- This assumes variables are represented in a certain way, and | ||
| -- its existence seems a bit sketchy |
There was a problem hiding this comment.
It is likely used for doing a bit of introspection. Might not be needed if the information is available by other means.
| valueInt :: r (Value r) -> Maybe Integer | ||
| value :: r (Value r) -> Doc | ||
|
|
||
| -- Again, wh are we recreating all these list functions? |
There was a problem hiding this comment.
There is likely a single language that needs a "middle layer" to intercept these, tweak, and pass on.
| class VisibilityElim r where | ||
| visibility :: r (Visibility r) -> Doc | ||
|
|
||
| -- Why do we need a Doc here? |
There was a problem hiding this comment.
Only the instance can tell!
|
@bmaclach if you're listening and can take the time, would you be able to weigh in on some of the questions I raised? Finding the PRs and issues related to certain decisions has proven quite difficult, and if you have any insight it would be super helpful. |
2 participants
In #4398 @TOTBWF hinted at some deeper grammatical and architectural issues with GOOL that may cause problems with #4360. In order to (hopefully) bring more clarity to this issue, I opted to add comments to some of the areas that I believe he was referring to.
The issues fall into a few categories:
LanguagePolymorphicis representative of a lot of the backend of GOOL, where we often unwrap our functions down toDocorString, make some changes, then wrap them back up intoValue,Variable, etc. I get the sense this back-and-forth is potentially problematic in general, but it is especially so here because it assumes each renderer uses the same representation - an assumption that our refactor will violate. I think we can still emulate that assumption, but it might get messy.YElimclasses in theRendererClassesXfiles are related to the above point. I'm not actually 100% sure if they're a bad thing as such; the bigger issue might be that they're used throughout the translation process instead of just at the end.RendererClassesXhave to do with classes that seem to have almost exactly the same signature as classes inInterfaceX. My understanding is that they exist as simpler-to-implement methods that functions inLanguagePolymorphiccan call to implement theInterfaceXmethods. This works, but creates difficulties when debugging (since the actual implementations of many methods ends up being several calls deep). It also often relies on the representation of language constructs, as I mentioned in theLanguagePolymorphicpoint.RendererClassesXthat belong inInterfaceX, and vice-versa.It seems like we need to parse through some of this in order to find our next steps. Ideally we could fix any blockers for the refactor in a separate PR, and then continue with the refactor on its branch. I'm still not entirely clear on which issues are actual blockers and which are simply code smells or even stylistic choices. If @TOTBWF and @JacquesCarette could take a look and give direction for a second pass of commenting, that might be a good place to start.